Feat: Add tls_bridge.upstream_insecure to skip origin cert verification#677
Feat: Add tls_bridge.upstream_insecure to skip origin cert verification#677aslom wants to merge 1 commit into
Conversation
The TLS bridge re-originates to the real origin via NewUpstreamClient, which verified against system roots + upstream_ca_bundle and never skipped verification. For internal/self-signed upstreams (e.g. a private LiteLLM endpoint) where the origin CA is unavailable, add an opt-in escape hatch: - NewUpstreamClient(extraRootsPEM, insecure): sets InsecureSkipVerify when insecure is true. - TLSBridgeConfig.UpstreamInsecure (yaml: upstream_insecure): plumbs the flag from config; authbridge-proxy logs a warning when enabled. - upstream_test.go: cover insecure=true accepting a self-signed origin. Prefer upstream_ca_bundle when the origin CA is available. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Aleksander Slominski <aslom@us.ibm.com>
📝 WalkthroughWalkthroughChangesThe TLS bridge adds Upstream TLS verification
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant authbridge-proxy
participant TLSBridgeConfig
participant NewUpstreamClient
participant UpstreamOrigin
authbridge-proxy->>TLSBridgeConfig: read UpstreamInsecure
authbridge-proxy->>NewUpstreamClient: pass CA bundle and insecure flag
NewUpstreamClient->>UpstreamOrigin: initiate TLS connection
UpstreamOrigin-->>NewUpstreamClient: present origin certificate
NewUpstreamClient-->>authbridge-proxy: return configured HTTP client
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@huang195 I am not sure if it is the right solution - I tried to describe the problem there may be better way to do it - option to (pre)load self-signed cert for LiteLLM? Have them inluded in rossocortex ? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/cmd/authbridge-proxy/main.go`:
- Around line 332-338: Replace the informational log.Printf call in the
cfg.TLSBridge.UpstreamInsecure branch with slog.Warn, preserving the existing
warning message and condition. Keep the adjacent log.Fatalf error handling
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7c6e8d5-18fc-4808-92f4-9e3d87e90d1e
📒 Files selected for processing (4)
authbridge/authlib/config/config.goauthbridge/authlib/tlsbridge/upstream.goauthbridge/authlib/tlsbridge/upstream_test.goauthbridge/cmd/authbridge-proxy/main.go
| up, uerr := tlsbridge.NewUpstreamClient(extra, cfg.TLSBridge.UpstreamInsecure) | ||
| if uerr != nil { | ||
| log.Fatalf("tls-bridge upstream client failed: %v", uerr) | ||
| } | ||
| if cfg.TLSBridge.UpstreamInsecure { | ||
| log.Printf("tls-bridge: upstream_insecure=true — re-origination does NOT verify the upstream TLS cert") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use log/slog for the insecure warning.
The new log statement uses log.Printf, but per the coding guidelines, log/slog should be used for logging. Since this is an informational warning about reduced security, slog.Warn is the appropriate choice and matches the pattern used elsewhere in this file. Note that while boot-path log.Fatalf is preserved by design based on learnings, standard informational logging should still use slog.
♻️ Proposed fix
up, uerr := tlsbridge.NewUpstreamClient(extra, cfg.TLSBridge.UpstreamInsecure)
if uerr != nil {
log.Fatalf("tls-bridge upstream client failed: %v", uerr)
}
if cfg.TLSBridge.UpstreamInsecure {
- log.Printf("tls-bridge: upstream_insecure=true — re-origination does NOT verify the upstream TLS cert")
+ slog.Warn("tls-bridge: upstream_insecure=true — re-origination does NOT verify the upstream TLS cert")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| up, uerr := tlsbridge.NewUpstreamClient(extra, cfg.TLSBridge.UpstreamInsecure) | |
| if uerr != nil { | |
| log.Fatalf("tls-bridge upstream client failed: %v", uerr) | |
| } | |
| if cfg.TLSBridge.UpstreamInsecure { | |
| log.Printf("tls-bridge: upstream_insecure=true — re-origination does NOT verify the upstream TLS cert") | |
| } | |
| up, uerr := tlsbridge.NewUpstreamClient(extra, cfg.TLSBridge.UpstreamInsecure) | |
| if uerr != nil { | |
| log.Fatalf("tls-bridge upstream client failed: %v", uerr) | |
| } | |
| if cfg.TLSBridge.UpstreamInsecure { | |
| slog.Warn("tls-bridge: upstream_insecure=true — re-origination does NOT verify the upstream TLS cert") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@authbridge/cmd/authbridge-proxy/main.go` around lines 332 - 338, Replace the
informational log.Printf call in the cfg.TLSBridge.UpstreamInsecure branch with
slog.Warn, preserving the existing warning message and condition. Keep the
adjacent log.Fatalf error handling unchanged.
Sources: Coding guidelines, Learnings
2 participants
Summary
Adds an opt-in
tls_bridge.upstream_insecureflag. When the TLS bridgere-originates a decrypted request to the real origin,
upstream_insecure: truesets
InsecureSkipVerifyon the re-origination client so it does not verifythe origin's certificate. Default is unchanged (verify against system roots +
upstream_ca_bundle).NewUpstreamClient(extraRootsPEM, insecure)— newinsecureparameter.TLSBridgeConfig.UpstreamInsecure(yaml: upstream_insecure) — plumbs it;authbridge-proxylogs a warning when enabled.insecure=trueaccepting a self-signed origin.Why this is needed (the rossocortex / rossoctlx use case)
rossocortexis a local budget/credential-injection proxy that sits in front of an
LLM endpoint, and
rossoctlxis its CLI. rossocortex embeds AuthBridge (proxy-sidecar mode) as its
outbound TLS bridge: agent traffic is forwarded through AuthBridge so its
plugins (
litellm-budget-track,inference-parser,placeholder-resolve)can see and act on the request, and AuthBridge's
tls_bridgeterminates theoutbound TLS to do it.
The real deployments this targets point at an internal LiteLLM endpoint whose
certificate chain is signed by a private/enterprise CA (e.g.
ete-litellm.ai-models.vpc-int.res.ibm.com). When AuthBridge's TLS bridgere-originates to that endpoint,
NewUpstreamClientverifies the origin againstsystem roots (+ any
upstream_ca_bundle). The private CA is not in the systemtrust store, so verification fails with:
Today that failure forces the bridge into passthrough for the host — the
request tunnels un-inspected, so AuthBridge's budget/parser plugins never see
it. In other words, without a way to trust (or skip verifying) the private
upstream, AuthBridge cannot own the connection to an internal LiteLLM, which
defeats the reason rossocortex embeds it.
upstream_insecuregives operators of such internal endpoints a documentedescape hatch: AuthBridge can complete re-origination to the self-signed upstream
and keep the request on the inspected path. rossoctlx exposes this end-to-end so
users never edit AuthBridge YAML by hand:
ROSSOCORTEX_UPSTREAM_INSECURE=1is the single user-facing switch.start.tls_bridge.upstream_insecure: trueinto the per-agentAuthBridge config it generates.
This mirrors the pattern operators already expect from
curl -k,GIT_SSL_NO_VERIFY,PGSSLMODE=require, etc. — a deliberate, logged,opt-in relaxation for private infrastructure, off by default.
Why not just use
upstream_ca_bundle?upstream_ca_bundleis the preferred, secure option when the private CA isavailable as a PEM file — and the flag docs/commit say so explicitly. It is
not always practical in the rossocortex context:
friction users skip — and then they silently fall back to passthrough.
So
upstream_insecureis the low-friction complement, not a replacement.What changed
authlib/tlsbridge/upstream.goNewUpstreamClient(extraRootsPEM, insecure)setsInsecureSkipVerify: insecureauthlib/config/config.goTLSBridgeConfig.UpstreamInsecure(yaml: upstream_insecure)cmd/authbridge-proxy/main.gocfg.TLSBridge.UpstreamInsecure; warns when enabledauthlib/tlsbridge/upstream_test.goinsecure=trueaccepts a self-signed origin;falsestill rejectsConfig example:
Test plan
go test ./authlib/tlsbridge/ -run TestUpstreamClient(new insecure case passes;falsestill rejects self-signed).go build ./authlib/... && CGO_ENABLED=0 GOOS=linux go build ./cmd/authbridge-proxy/.ROSSOCORTEX_UPSTREAM_INSECURE=1 rossoctlx cortex start --upstream https://<internal-litellm>; an agent LLM call returns 200 instead of a 502 self-signed error.Suggested improvements / follow-ups
Scope insecure per host, not globally. Today it's a single bool for the
whole bridge. A
upstream_insecure_hosts: [glob, …](mirroringpassthrough_hosts) would let the internal LiteLLM be trusted-insecure whileevery other origin still verifies — much smaller blast radius.
Prefer
upstream_ca_bundleauto-discovery. rossocortex actually ships a288 KB
combined-ca.pemthat does verify the internal endpoint on a directsocket. A cleaner long-term fix is to feed that bundle as
upstream_ca_bundleso
upstream_insecureisn't needed at all. Worth documenting the decisiontree: CA bundle > per-host insecure > global insecure.
Investigate the
bad record MACMITM handshake. In the rossocortexintegration, AuthBridge's bridge handshake with the Python
httpxclientintermittently fails (
tls: bad record MAC) and falls back to passthrough —so even with
upstream_insecure, AuthBridge doesn't reliably keep therequest on the inspected path. Likely an ALPN/h2 interaction in the minted
leaf or terminator. Fixing this is what makes
upstream_insecureactuallydeliver inspected traffic rather than just unblocking passthrough.
Surface it in observability. Emit a session/stat event (or a one-time
startup metric) when re-origination runs insecure, so an operator auditing a
deployment can see that verification is disabled without grepping logs.
Validation guardrail. Consider making
upstream_insecure: true+upstream_ca_bundleset at the same time a validation warning (CA bundlewins / insecure is redundant), and refuse
upstream_insecureunlesstls_bridge.mode: enabled.Naming parity. If the envoy-sidecar path grows an equivalent, keep the
field name identical so the two deployment shapes share one vocabulary.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes